Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker)

go/lib/infra/modules/segfetcher/fetcher.go, line 82 at r1 (raw file):

// FetchSegs fetches the required segments to build a path between src and dst
// of the request. Firs the request is validated and then depending on the

s/Firs/First

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

			continue
		}
		r := f.ReplyHandler.Handle(ctx, reply.Reply, reply.Peer, nil)

the nil channel here will cause a panic in SegReplyHandler

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

		select {
		case <-r.FullReplyProcessed():
			if err := r.Err(); err != nil {

This means one segment that fails to verify will break segment fetching, no?
(e.g. timeout when fetching a missing certificate)

go/lib/infra/modules/segfetcher/fetcher_test.go, line 117 at r1 (raw file):

			ExpectedSegs:   segfetcher.Segments{Up: seg.Segments{tg.seg130_111}},
		},
		// XXX(lukedirtwalker): testing the full loop is quite involved, not

hmm. Yeah.
TBH I don't think it is worth it, since Fetcher mostly just combines different components.
It's really nasty to test, and if it is buggy it will immediately show when sciond/PS are unable to provide paths.

go/lib/infra/modules/segfetcher/segreplyhandler.go, line 164 at r1 (raw file):

		full:  make(chan struct{}),
	}
	verifiedCh, units := h.Verifier.Verify(ctx, reply, server)

hm.
This changes the crypto lookup strategy.
Which is correct for the PS, but I remember some discussion of sciond going to CS instead.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @oncilla)

go/lib/infra/modules/segfetcher/fetcher.go, line 82 at r1 (raw file):

Done.

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

No? How? Selecting over a nil channel is valid, it will never be selected.

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

Yes. That is the current behavior as well. If we have multiple segments for the same type (e.g. multiple core segs) it could make sense to ignore the error and we might still get paths.
I don't want to overcomplicate things now.

go/lib/infra/modules/segfetcher/fetcher_test.go, line 117 at r1 (raw file):

Yeah. I will leave the comment so that it is clear why it isn't done.

go/lib/infra/modules/segfetcher/segreplyhandler.go, line 164 at r1 (raw file):

Yeah I had to change it for the PS. But forgot to make it configurable for sciond. Done.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

oh, sorry I was looking at:

and somehow thought that earlyTrigger is closed instead of result.eraly.

nevermind.

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

ack